feat: add handling for HTML and empty responses from providers #1105
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
📝 WalkthroughWalkthroughAdds HTML-detection and extraction utilities, explicit empty/HTML response handling across multiple providers, a generic concurrent response handler, tests/benchmarks for HTML handling, two new provider error constants, small UI tweak, and changelog updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Points to review closely:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
core/providers/utils/utils.go (1)
608-618: Consider precompiling regex patterns for better performance.The regex patterns for h1/h2/h3 tags are compiled on every function call. Since
ExtractHTMLErrorMessagemay be called in error paths for provider responses, precompiling these as package-level variables would reduce allocation overhead.+var ( + h1Regex = regexp.MustCompile(`(?i)<h1[^>]*>([^<]+)</h1>`) + h2Regex = regexp.MustCompile(`(?i)<h2[^>]*>([^<]+)</h2>`) + h3Regex = regexp.MustCompile(`(?i)<h3[^>]*>([^<]+)</h3>`) + metaDescRe = regexp.MustCompile(`(?i)<meta\s+name="description"\s+content="([^"]+)"`) + scriptStyleRe = regexp.MustCompile(`(?i)<script[^>]*>.*?</script>|<style[^>]*>.*?</style>`) + htmlTagRe = regexp.MustCompile(`<[^>]+>`) +)core/providers/utils/html_response_handler_test.go (4)
10-80: Well-structured test coverage for HTML detection.The table-driven tests cover the main scenarios effectively, including Content-Type detection, body inspection, and negative cases.
Consider adding a test case for nil response to verify the implementation's nil-safety:
+ { + name: "Nil response with HTML body", + contentType: "", + body: []byte("<!DOCTYPE html><html></html>"), + expectedIsHTML: true, + description: "Should detect HTML from body when response is nil", + },Then modify the test loop to handle nil response:
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - resp := &fasthttp.Response{} - resp.Header.Set("Content-Type", tt.contentType) + var resp *fasthttp.Response + if tt.contentType != "" { + resp = &fasthttp.Response{} + resp.Header.Set("Content-Type", tt.contentType) + } result := IsHTMLResponse(resp, tt.body)
82-192: Comprehensive test coverage for HTML error message extraction.The tests effectively validate the extraction priority chain and edge cases like empty headers and script/style filtering.
Consider adding a test case to verify long text truncation (implementation truncates at 200 characters):
+ { + name: "Truncate very long text", + htmlBody: []byte(` + <html> + <body> + <div>` + strings.Repeat("This is a very long error message that should be truncated. ", 10) + `</div> + </body> + </html> + `), + expectMsg: "", // Would need to calculate expected truncated message + description: "Should truncate messages longer than 200 characters", + },
241-247: Verify that errorResp is populated for valid JSON case.The test case for valid JSON should verify that the
errorRespvariable was actually populated with the parsed data, not just check for an empty message.Apply this diff to strengthen the assertion:
{ name: "Valid JSON error response", statusCode: 400, contentType: "application/json", body: []byte(`{"error": {"message": "Invalid request"}, "code": "invalid_request"}`), description: "Should handle valid JSON without HTML detection", expectedInMessage: "", },Then in the test loop after line 273:
// Check if expected message is in the response if tt.expectedInMessage != "" && !strings.Contains(bifrostErr.Error.Message, tt.expectedInMessage) { t.Errorf("Expected message to contain %q, got %q", tt.expectedInMessage, bifrostErr.Error.Message) } + + // For valid JSON case, verify errorResp was populated + if tt.name == "Valid JSON error response" { + if errorResp["code"] != "invalid_request" { + t.Errorf("Expected errorResp to contain parsed JSON, got %+v", errorResp) + } + }
194-282: Add test case for empty response body handling.The implementation explicitly handles empty responses (see
core/providers/utils/utils.golines 337-346), but this scenario is not tested.Add this test case to verify empty body handling:
{ + name: "Empty response body", + statusCode: 500, + contentType: "application/json", + body: []byte(" "), + description: "Should handle empty/whitespace-only response body", + expectedInMessage: "provider returned empty response", + }, + { name: "Valid JSON error response", statusCode: 400, contentType: "application/json", body: []byte(`{"error": {"message": "Invalid request"}, "code": "invalid_request"}`), description: "Should handle valid JSON without HTML detection", expectedInMessage: "", },Note: Verify the exact error message constant value in
core/schemas/provider.go.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/changelog.md(1 hunks)core/providers/utils/html_response_handler_test.go(1 hunks)core/providers/utils/utils.go(5 hunks)core/providers/vertex/errors.go(2 hunks)core/schemas/provider.go(1 hunks)transports/changelog.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
core/changelog.mdtransports/changelog.mdcore/schemas/provider.gocore/providers/utils/html_response_handler_test.gocore/providers/vertex/errors.gocore/providers/utils/utils.go
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/schemas/provider.gocore/providers/utils/html_response_handler_test.gocore/providers/vertex/errors.gocore/providers/utils/utils.go
📚 Learning: 2025-12-15T10:16:21.909Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/providers/huggingface/huggingface_test.go:12-63
Timestamp: 2025-12-15T10:16:21.909Z
Learning: In provider tests under core/providers/<provider>/*_test.go, do not require or flag the use of defer for Shutdown(); instead call client.Shutdown() at the end of each test function. This pattern appears consistent across all provider tests. Apply this rule only within this path; for other tests or resources, defer may still be appropriate.
Applied to files:
core/providers/utils/html_response_handler_test.go
🧬 Code graph analysis (2)
core/providers/utils/html_response_handler_test.go (1)
core/providers/utils/utils.go (3)
IsHTMLResponse(551-589)ExtractHTMLErrorMessage(593-657)HandleProviderAPIError(321-379)
core/providers/vertex/errors.go (4)
core/utils.go (1)
Ptr(56-58)core/schemas/utils.go (1)
Ptr(16-18)core/schemas/provider.go (2)
ErrProviderResponseEmpty(31-31)Provider(314-361)core/providers/utils/utils.go (2)
IsHTMLResponse(551-589)ExtractHTMLErrorMessage(593-657)
🔇 Additional comments (8)
core/changelog.md (1)
1-1: LGTM!Changelog entry accurately describes the feature being added.
transports/changelog.md (1)
1-1: LGTM!Changelog entry is consistent with the core changelog and appropriately documents the feature addition.
core/schemas/provider.go (1)
31-32: LGTM!The new error constants follow the existing naming convention and provide clear, descriptive messages. They are logically placed with the other provider error constants.
core/providers/utils/utils.go (4)
387-397: LGTM on empty response handling.The early exit for empty responses with a clear error message improves debugging. The
IsBifrostError: trueflag appropriately marks this as a Bifrost-level issue rather than a provider API error.
436-446: LGTM on HTML fallback handling.The HTML detection as a fallback after JSON failure is a good optimization - expensive regex only runs when JSON parsing fails. The error structure appropriately distinguishes between the error type (
ErrProviderResponseHTML) and the extracted message.
549-589: LGTM on HTML detection logic.The layered approach (Content-Type header first, then body inspection) is well-designed. The 20-byte threshold for body inspection is a sensible optimization. The function correctly handles nil
respparameter for cases where only body-based detection is needed.
348-356: No changes needed. The function's behavior of returning an emptyErrorFieldon successful JSON parsing is intentional design that serves a clear API contract: when JSON parsing succeeds, the provider-specific error details are already populated in theerrorRespparameter (passed by reference tosonic.Unmarshal), and callers are expected to use those fields directly. This pattern is validated by tests (which explicitly expect emptyexpectedInMessagefor valid JSON) and consistently followed by all callers (cohere, anthropic, gemini, elevenlabs, openai, anthropic-batch), which all extract error details from the provider-specificerrorRespstruct after calling this function. The emptyErrorFieldsignals parse success and directs callers to the appropriate data source.Likely an incorrect or invalid review comment.
core/providers/utils/html_response_handler_test.go (1)
284-302: Well-implemented benchmarks.Both benchmarks use appropriate test data and correctly reset the timer after setup. These will provide useful performance metrics for the HTML detection and extraction logic.
Pratham-Mishra04
force-pushed
the
12-16-feat_http_response_parsing_support_added
branch
from
60f9cb4 to
080f651
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/providers/utils/utils.go (1)
594-664: Pre-compile regular expressions at package level.The function compiles multiple regular expressions on every invocation, including one inside a loop (line 618). Since this runs on error paths that may occur frequently, pre-compiling these regexes as package-level variables will improve performance.
Apply this pattern:
var ( headerTagsRegex = regexp.MustCompile(`(?i)<(h[123])[^>]*>([^<]+)</\1>`) metaDescRegex = regexp.MustCompile(`(?i)<meta\s+name="description"\s+content="([^"]+)"`) scriptStyleRegex = regexp.MustCompile(`(?i)<script[^>]*>.*?</script>|<style[^>]*>.*?</style>`) htmlTagRegex = regexp.MustCompile(`<[^>]+>`) ) func ExtractHTMLErrorMessage(body []byte) string { if len(body) > maxBodySize { body = body[:maxBodySize] } bodyStr := string(body) // ... rest of function using pre-compiled regexesThen replace the inline
regexp.MustCompilecalls with the pre-compiled variables.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/changelog.md(1 hunks)core/providers/utils/html_response_handler_test.go(1 hunks)core/providers/utils/utils.go(5 hunks)core/providers/vertex/errors.go(2 hunks)core/schemas/provider.go(1 hunks)transports/changelog.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- transports/changelog.md
- core/providers/utils/html_response_handler_test.go
- core/schemas/provider.go
- core/changelog.md
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
core/providers/vertex/errors.gocore/providers/utils/utils.go
🧠 Learnings (1)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/providers/vertex/errors.gocore/providers/utils/utils.go
🧬 Code graph analysis (2)
core/providers/vertex/errors.go (4)
core/schemas/bifrost.go (3)
BifrostError(432-441)ErrorField(450-457)BifrostErrorExtraFields(499-503)core/utils.go (1)
Ptr(56-58)core/schemas/utils.go (1)
Ptr(16-18)core/schemas/provider.go (2)
ErrProviderResponseEmpty(31-31)Provider(314-361)
core/providers/utils/utils.go (3)
core/schemas/bifrost.go (2)
BifrostError(432-441)ErrorField(450-457)ui/lib/types/logs.ts (2)
BifrostError(226-232)ErrorField(217-224)core/schemas/provider.go (2)
ErrProviderResponseEmpty(31-31)ErrProviderResponseHTML(32-32)
🔇 Additional comments (5)
core/providers/vertex/errors.go (2)
34-52: LGTM! Empty response handling is correct.The empty response check properly handles the nil meta case and returns an appropriate error message. The nil-safe pattern used here aligns well with the rest of the function.
54-72: LGTM! HTML detection is properly positioned.The HTML response check correctly runs before JSON parsing attempts, which prevents unnecessary parsing errors. The nil-safe meta handling is consistent with the empty response block.
core/providers/utils/utils.go (3)
318-379: LGTM! Efficient error handling flow.The optimization strategy of checking JSON before HTML is well-documented and sound. The empty response check prevents unnecessary processing, and the fallback chain (JSON → HTML → raw) handles all cases gracefully.
387-493: LGTM! Early empty check optimizes performance.The empty response check before spawning goroutines prevents unnecessary concurrent processing. The HTML detection fallback is correctly positioned after JSON parsing fails.
549-589: LGTM! Well-optimized HTML detection.The detection strategy is efficient: Content-Type header first, then size check, then body inspection. Using string search instead of regex for the indicator list is a good performance choice.
Pratham-Mishra04
force-pushed
the
12-16-feat_http_response_parsing_support_added
branch
from
080f651 to
de2743a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
core/providers/utils/utils.go (3)
436-455: Inconsistent error message pattern betweenHandleProviderAPIErrorandHandleProviderResponse.In
HandleProviderAPIError(line 365), the extracted HTML message is used directly asMessage. Here,Messageis set to the constantschemas.ErrProviderResponseHTMLwhile the extracted message is placed in theErrorfield. This inconsistency could confuse consumers expecting uniform error structures.Consider aligning with
HandleProviderAPIError:if IsHTMLResponse(nil, responseBody) { errorMessage := ExtractHTMLErrorMessage(responseBody) return nil, nil, &schemas.BifrostError{ IsBifrostError: false, Error: &schemas.ErrorField{ - Message: schemas.ErrProviderResponseHTML, - Error: errors.New(errorMessage), + Message: errorMessage, }, } }
591-664: Consider pre-compiling regex patterns for performance.The regexes in
ExtractHTMLErrorMessageare compiled on every invocation. While themaxBodySizeguard mitigates ReDoS concerns, pre-compiling these patterns at package initialization would improve performance for high-throughput scenarios.+// Pre-compiled regexes for HTML extraction (avoid recompilation on each call) +var ( + headerTagRe = regexp.MustCompile(`(?i)<(h[1-3])[^>]*>([^<]+)</\1>`) + metaDescRe = regexp.MustCompile(`(?i)<meta\s+name="description"\s+content="([^"]+)"`) + scriptStyleRe = regexp.MustCompile(`(?is)<script[^>]*>.*?</script>|<style[^>]*>.*?</style>`) + htmlTagRe = regexp.MustCompile(`<[^>]+>`) +) // Limit body size to prevent ReDoS on very large malicious responses const maxBodySize = 32 * 1024 // 32KBThen use the pre-compiled regexes in the function:
// Instead of: // re := regexp.MustCompile("(?i)" + pattern) // Use: // matches := headerTagRe.FindStringSubmatch(bodyStr)
616-625: Minor: Header tag regex could be more efficient.The current approach compiles a new regex for each tag (
h1,h2,h3). A single pre-compiled regex with a capture group for the tag name would reduce overhead:// Single regex that matches h1, h2, or h3 headerTagRe := regexp.MustCompile(`(?i)<(h[1-3])[^>]*>([^<]+)</\1>`) if matches := headerTagRe.FindStringSubmatch(bodyStr); len(matches) > 2 { msg := strings.TrimSpace(matches[2]) if msg != "" { return msg } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/changelog.md(1 hunks)core/providers/utils/html_response_handler_test.go(1 hunks)core/providers/utils/utils.go(5 hunks)core/providers/vertex/errors.go(2 hunks)core/schemas/provider.go(1 hunks)transports/changelog.md(1 hunks)ui/app/workspace/providers/views/modelProviderKeysTableView.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- core/providers/utils/html_response_handler_test.go
- core/schemas/provider.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
core/providers/utils/utils.gotransports/changelog.mdcore/providers/vertex/errors.gocore/changelog.mdui/app/workspace/providers/views/modelProviderKeysTableView.tsx
🧠 Learnings (1)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/providers/utils/utils.gocore/providers/vertex/errors.go
🧬 Code graph analysis (2)
core/providers/utils/utils.go (3)
core/schemas/bifrost.go (2)
BifrostError(432-441)ErrorField(450-457)ui/lib/types/logs.ts (2)
BifrostError(226-232)ErrorField(217-224)core/schemas/provider.go (2)
ErrProviderResponseEmpty(31-31)ErrProviderResponseHTML(32-32)
core/providers/vertex/errors.go (4)
core/schemas/bifrost.go (4)
BifrostError(432-441)ErrorField(450-457)BifrostErrorExtraFields(499-503)RequestType(85-85)core/utils.go (1)
Ptr(56-58)core/schemas/provider.go (2)
ErrProviderResponseEmpty(31-31)Provider(314-361)core/providers/utils/utils.go (2)
IsHTMLResponse(551-589)ExtractHTMLErrorMessage(596-664)
🔇 Additional comments (10)
transports/changelog.md (1)
1-1: LGTM!Changelog entry is clear and accurately describes the feature addition.
core/changelog.md (1)
1-1: LGTM!Changelog entry appropriately documents the new HTML and empty response handling feature in core.
core/providers/vertex/errors.go (2)
34-52: LGTM!Empty response handling is correctly implemented with nil-safe access to
metafields. The whitespace trimming ensures that responses containing only whitespace are treated as empty.
54-72: LGTM!HTML response detection is properly placed before JSON parsing to avoid unnecessary unmarshal attempts on HTML error pages. The nil-safe meta access pattern is consistent with the empty response block.
core/providers/utils/utils.go (5)
14-14: LGTM!Import of
regexppackage is necessary for the HTML extraction logic.
336-346: LGTM!Empty response check using trimmed body is appropriate—this catches responses with only whitespace characters.
348-378: Well-structured optimization.Trying JSON parsing first before falling back to HTML detection is the right approach—most responses will be valid JSON, so this avoids expensive regex operations on the hot path.
549-589: LGTM!
IsHTMLResponsecorrectly handles the case whenrespis nil (skips Content-Type check) and uses a reasonable set of HTML indicators. The early return for bodies < 20 bytes is a sensible optimization.
388-397: LGTM!Early empty response check in
HandleProviderResponsecorrectly returns before spawning concurrent goroutines, avoiding unnecessary work.ui/app/workspace/providers/views/modelProviderKeysTableView.tsx (1)
135-152: LGTM!The
size="md"prop addition and the simplified ternary in the map callback are clean improvements. The logic remains unchanged while improving readability.
Pratham-Mishra04
force-pushed
the
12-16-feat_http_response_parsing_support_added
branch
from
de2743a to
663a943
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
core/providers/utils/utils.go (2)
385-447: HTML error from JSON handlers: consider surfacing extracted message inMessageas well.When
HandleProviderResponsedetects HTML (IsHTMLResponse(nil, responseBody)), it returns aBifrostErrorwhoseError.Messageisschemas.ErrProviderResponseHTMLand stashes the extracted HTML text inError.Error. SinceError.Errorisjson:"-", external consumers only see the generic message.If you want clients to see the provider’s text while still signaling the HTML case, consider either:
- Putting the extracted HTML text into
Message(and usingErrProviderResponseHTMLasError.Codeor viaType), or- Adding the extracted message into
ExtraFieldswhile keeping the genericMessage.This keeps programmatic detection via
ErrProviderResponseHTMLbut improves debuggability.
550-665: HTML detection/extraction helpers are robust; regex precompilation would be a minor win.
IsHTMLResponseandExtractHTMLErrorMessagecombine header checks, body heuristics, a 32KB truncation guard, and targeted tag/meta extraction, which is a good balance between safety and usefulness. The ReDoS concern from earlier reviews is addressed bymaxBodySize.As a minor improvement, you could precompile the regex patterns (
h1/h2/h3, meta description, script/style removal, generic tag stripping) as package-level variables to avoid recompilation on each call, while keeping the same behavior.core/providers/elevenlabs/elevenlabs.go (1)
525-535: Empty Elevenlabs transcription response guard is correct; consider adding context fields.The new trimmed-empty check avoids trying to parse a missing transcription and correctly returns
ErrProviderResponseEmptyas a Bifrost error.Optionally, you might also populate
ExtraFields(e.g.,Provider,RequestType: schemas.TranscriptionRequest, andModelRequested) so downstream logging/UX has the same context as other provider error paths.core/providers/openai/openai.go (1)
1893-1927: Transcription empty/HTML handling is correct; consider enriching the HTML error context.The new
responseBodytrimming/empty check and the HTML detection on JSON unmarshal failure both look good and align with the shared utilities (IsHTMLResponse,ExtractHTMLErrorMessage). Using a copied body for both parsing and raw-response decoding is also a nice touch.For the HTML case, you currently return a
BifrostErrorwith onlyError.Messageset to the extracted HTML text. It may be helpful to also setStatusCodeandExtraFields(e.g.,Provider,RequestType: schemas.TranscriptionRequest,ModelRequested) so logs and clients can attribute the error to the originating call more easily, similar to other error paths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
core/changelog.md(1 hunks)core/providers/elevenlabs/elevenlabs.go(1 hunks)core/providers/mistral/mistral.go(5 hunks)core/providers/openai/openai.go(2 hunks)core/providers/utils/html_response_handler_test.go(1 hunks)core/providers/utils/utils.go(6 hunks)core/providers/vertex/errors.go(2 hunks)core/schemas/provider.go(1 hunks)transports/changelog.md(1 hunks)ui/app/workspace/providers/views/modelProviderKeysTableView.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- core/schemas/provider.go
- core/providers/utils/html_response_handler_test.go
- transports/changelog.md
- ui/app/workspace/providers/views/modelProviderKeysTableView.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
core/providers/elevenlabs/elevenlabs.gocore/providers/openai/openai.gocore/providers/mistral/mistral.gocore/providers/utils/utils.gocore/providers/vertex/errors.gocore/changelog.md
🧠 Learnings (3)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/providers/elevenlabs/elevenlabs.gocore/providers/openai/openai.gocore/providers/mistral/mistral.gocore/providers/utils/utils.gocore/providers/vertex/errors.go
📚 Learning: 2025-12-11T11:58:25.307Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: core/providers/openai/responses.go:42-84
Timestamp: 2025-12-11T11:58:25.307Z
Learning: In core/providers/openai/responses.go (and related OpenAI response handling), document and enforce the API format constraint: if ResponsesReasoning != nil and the response contains content blocks, all content blocks should be treated as reasoning blocks by default. Implement type guards or parsing logic accordingly, and add unit tests to verify that when ResponsesReasoning is non-nil, content blocks are labeled as reasoning blocks. Include clear comments in the code explaining the rationale and ensure downstream consumers rely on this behavior.
Applied to files:
core/providers/openai/openai.go
📚 Learning: 2025-12-14T14:43:30.902Z
Learnt from: Radheshg04
Repo: maximhq/bifrost PR: 980
File: core/providers/openai/images.go:10-22
Timestamp: 2025-12-14T14:43:30.902Z
Learning: Enforce the OpenAI image generation SSE event type values across the OpenAI image flow in the repository: use "image_generation.partial_image" for partial chunks, "image_generation.completed" for the final result, and "error" for errors. Apply this consistently in schemas, constants, tests, accumulator routing, and UI code within core/providers/openai (and related Go files) to ensure uniform event typing and avoid mismatches.
Applied to files:
core/providers/openai/openai.go
🧬 Code graph analysis (3)
core/providers/elevenlabs/elevenlabs.go (3)
core/schemas/bifrost.go (2)
BifrostError(432-441)ErrorField(450-457)ui/lib/types/logs.ts (2)
BifrostError(226-232)ErrorField(217-224)core/schemas/provider.go (1)
ErrProviderResponseEmpty(31-31)
core/providers/openai/openai.go (3)
core/schemas/bifrost.go (2)
BifrostError(432-441)ErrorField(450-457)core/schemas/provider.go (2)
ErrProviderResponseEmpty(31-31)ErrProviderResponseUnmarshal(30-30)core/providers/utils/utils.go (4)
IsHTMLResponse(552-590)ExtractHTMLErrorMessage(597-665)NewBifrostOperationError(733-744)ShouldSendBackRawResponse(784-789)
core/providers/utils/utils.go (3)
core/schemas/bifrost.go (2)
BifrostError(432-441)ErrorField(450-457)ui/lib/types/logs.ts (2)
BifrostError(226-232)ErrorField(217-224)core/schemas/provider.go (2)
ErrProviderResponseEmpty(31-31)ErrProviderResponseHTML(32-32)
🔇 Additional comments (8)
core/providers/utils/utils.go (4)
14-14:regexpimport usage is appropriate.The new
regexpimport is only used in the HTML error extraction helper below and is scoped to error-handling paths, which is appropriate.
315-379: Layered provider API error handling (JSON → HTML → raw) looks solid.The new flow in
HandleProviderAPIError—decode withCheckAndDecodeBody, explicitly guard empty bodies, attempt JSON unmarshal first, then fall back to HTML detection and finally raw body—is a clear improvement and keeps the expensive HTML path off the hot JSON path.
495-503:ParseAndSetRawRequesthelper and comment are clear.The clarified comment and helper encapsulation around parsing and attaching
RawRequestare straightforward and reduce duplication at call sites.
539-548: Centralized content-encoding handling viaCheckAndDecodeBodyis a good consolidation.Using
CheckAndDecodeBodyas the single place that interpretsContent-Encoding(gzip vs. identity) simplifies all the call sites and makes it easier to extend if additional encodings appear later.core/providers/openai/openai.go (1)
2548-2551: Usingresp.Header.ContentType()is appropriate here.Switching to
resp.Header.ContentType()to derive the content type forFileContentis idiomatic and preserves the existing fallback toapplication/octet-streamwhen absent.core/providers/mistral/mistral.go (2)
302-319: Decode + empty-response guard in Mistral transcription looks good.Using
CheckAndDecodeBodyfollowed by a trimmed empty check protectsparseTranscriptionResponsefrom being fed an empty body and correctly surfacesErrProviderResponseEmptyas a Bifrost error.
470-575:processTranscriptionStreamEventhelper keeps streaming logic clean and consistent.The new
processTranscriptionStreamEventhelper and corresponding call sites handle:
- Early detection of Bifrost-shaped error payloads (with proper
ExtraFields),- Parsing of Mistral’s transcription stream data into typed events,
- Population of
BifrostTranscriptionStreamResponsewith chunk index and latency,- Marking the stream end on the “done” event and setting final latency,
which aligns with the streaming patterns used elsewhere in the codebase.
core/providers/vertex/errors.go (1)
4-72: Vertex error handling: good pre-parse empty/HTML checks with nil-safe metadata.Adding
stringsfor trimming, then:
- Guarding on an empty decoded body with
ErrProviderResponseEmpty, and- Checking for HTML error pages via
IsHTMLResponse/ExtractHTMLErrorMessagebefore any JSON parsing,provides clearer errors and avoids unnecessary JSON attempts. In both cases you now only populate
ExtraFieldswhenmetais non-nil, which prevents the nil-dereference risk called out in earlier reviews while still attaching provider/model/request context when available.
Pratham-Mishra04
force-pushed
the
12-16-feat_http_response_parsing_support_added
branch
from
663a943 to
332bc02
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/providers/mistral/mistral.go (1)
500-501: Good refactor for clarity.Renaming
processStreamEventtoprocessTranscriptionStreamEventmakes the function's purpose explicit and improves code readability.core/providers/utils/utils.go (1)
592-665: Robust HTML error extraction with proper safeguards.The function includes the recommended size limit (Line 598-600) to prevent ReDoS attacks and employs a sensible fallback strategy: title → headers → meta → visible text → generic message.
One micro-optimization: Lines 617-626 compile the same regex pattern inside the loop three times. Consider compiling once outside the loop:
// Try to extract from h1, h2, h3 tags (common for error pages) for _, tag := range []string{"h1", "h2", "h3"} { pattern := fmt.Sprintf("<%s[^>]*>([^<]+)</%s>", tag, tag) re := regexp.MustCompile("(?i)" + pattern) if matches := re.FindStringSubmatch(bodyStr); len(matches) > 1 { msg := strings.TrimSpace(matches[1]) if msg != "" { return msg } } }Could be:
// Try to extract from h1, h2, h3 tags (common for error pages) pattern := `(?i)<(h[1-3])[^>]*>([^<]+)</\1>` re := regexp.MustCompile(pattern) if matches := re.FindStringSubmatch(bodyStr); len(matches) > 2 { msg := strings.TrimSpace(matches[2]) if msg != "" { return msg } }However, since this only runs on error responses (not a hot path), the current implementation is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/changelog.md(1 hunks)core/providers/elevenlabs/elevenlabs.go(1 hunks)core/providers/mistral/mistral.go(5 hunks)core/providers/openai/openai.go(2 hunks)core/providers/utils/utils.go(6 hunks)transports/changelog.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- core/providers/openai/openai.go
- core/providers/elevenlabs/elevenlabs.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
core/providers/mistral/mistral.gocore/changelog.mdcore/providers/utils/utils.gotransports/changelog.md
🧠 Learnings (1)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/providers/mistral/mistral.gocore/providers/utils/utils.go
🧬 Code graph analysis (2)
core/providers/mistral/mistral.go (4)
core/providers/utils/utils.go (4)
CheckAndDecodeBody(540-548)NewBifrostOperationError(733-744)IsHTMLResponse(552-590)ExtractHTMLErrorMessage(597-665)core/schemas/provider.go (2)
ErrProviderResponseDecode(29-29)ErrProviderResponseEmpty(31-31)core/schemas/bifrost.go (2)
BifrostError(432-441)ErrorField(450-457)core/providers/mistral/types.go (1)
MistralTranscriptionResponse(53-59)
core/providers/utils/utils.go (3)
core/schemas/bifrost.go (2)
BifrostError(432-441)ErrorField(450-457)ui/lib/types/logs.ts (2)
BifrostError(226-232)ErrorField(217-224)core/schemas/provider.go (2)
ErrProviderResponseEmpty(31-31)ErrProviderResponseHTML(32-32)
🔇 Additional comments (6)
transports/changelog.md (1)
1-2: Verify changelog entries and maintain consistent verb tense.Line 1 accurately reflects the PR objective. However, Line 2 regarding "mistral transcription support" is not mentioned in the PR #1105 summary. Additionally, there's an inconsistency in verb tense: Line 1 uses imperative ("add") while Line 2 uses past tense ("added").
- Confirm whether the mistral transcription feature belongs in this changelog update or if it's from a separate stacked PR.
- If both entries should be present, align the verb tense for consistency. Conventional changelog format typically uses imperative/present tense (e.g., "add" instead of "added").
Consider updating to:
- feat: add handling for HTML and empty responses from providers - feat: added transcription support for mistral + feat: add handling for HTML and empty responses from providers + feat: add transcription support for mistralcore/changelog.md (1)
1-2: LGTM!The changelog entries are now properly separated and clearly document the new features. The previous issue of combining multiple features in a single line has been addressed.
core/providers/mistral/mistral.go (1)
302-333: Well-structured error handling flow.The response handling now follows a proper sequence: decode → empty-body check → JSON unmarshal → (on failure) HTML detection → fallback error. The HTML detection has been correctly moved to the unmarshal failure branch, addressing the previous review feedback.
core/providers/utils/utils.go (3)
321-379: Efficient error-handling priority cascade.The function correctly prioritizes cheap operations (decode, empty check, JSON parse) before expensive HTML detection. This design minimizes performance impact on the hot path (successful JSON responses).
387-493: Solid concurrent response handling with HTML fallback.The function efficiently handles responses by checking for empty bodies upfront, unmarshaling concurrently, and falling back to HTML detection only when JSON parsing fails. The HTML error wrapping (Line 442) uses
ErrProviderResponseHTMLas the message category with the extracted HTML text in theErrorfield, which provides clear error categorization.
550-590: Efficient HTML detection with minimal overhead.The function prioritizes fast checks (Content-Type header, body size) before scanning for HTML indicators using simple string matching. No regex is used here, keeping detection lightweight.
Pratham-Mishra04
force-pushed
the
12-16-feat_http_response_parsing_support_added
branch
from
332bc02 to
eeb6bc7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
core/providers/utils/utils.go (1)
616-626: Consider pre-compiling regex patterns for better performance.The regex patterns for extracting h1/h2/h3 content and meta descriptions are compiled on every call to
ExtractHTMLErrorMessage. Since this runs on error paths (not hot paths), performance impact is limited, but pre-compilation would be cleaner.+var ( + // Pre-compiled regex patterns for HTML extraction + headerRegex = map[string]*regexp.Regexp{ + "h1": regexp.MustCompile(`(?i)<h1[^>]*>([^<]+)</h1>`), + "h2": regexp.MustCompile(`(?i)<h2[^>]*>([^<]+)</h2>`), + "h3": regexp.MustCompile(`(?i)<h3[^>]*>([^<]+)</h3>`), + } + metaDescRegex = regexp.MustCompile(`(?i)<meta\s+name="description"\s+content="([^"]+)"`) + scriptStyleRegex = regexp.MustCompile(`(?i)<script[^>]*>.*?</script>|<style[^>]*>.*?</style>`) + htmlTagRegex = regexp.MustCompile(`<[^>]+>`) +)core/providers/utils/html_response_handler_test.go (1)
284-302: Consider adding a test forIsHTMLResponsewith nil response parameter.The
HandleProviderResponsefunction callsIsHTMLResponse(nil, responseBody). Adding a test case withresp = nilwould ensure body-only detection works correctly in isolation.{ name: "Nil response with HTML body", contentType: "", // won't be checked body: []byte("<!DOCTYPE html><html><body>Error</body></html>"), expectedIsHTML: true, description: "Should detect HTML from body when response is nil", },Then test with:
result := IsHTMLResponse(nil, tt.body)core/providers/mistral/mistral.go (1)
318-318: Redundant body copy.
responseBodyreturned fromCheckAndDecodeBodyis already a new slice (either fromBodyGunzip()orBody()). The explicit copy on line 318 is unnecessary.- copiedResponseBody := append([]byte(nil), responseBody...) - // Parse Mistral's transcription response var mistralResponse MistralTranscriptionResponse - if err := sonic.Unmarshal(copiedResponseBody, &mistralResponse); err != nil { - if providerUtils.IsHTMLResponse(resp, copiedResponseBody) { - errorMessage := providerUtils.ExtractHTMLErrorMessage(copiedResponseBody) + if err := sonic.Unmarshal(responseBody, &mistralResponse); err != nil { + if providerUtils.IsHTMLResponse(resp, responseBody) { + errorMessage := providerUtils.ExtractHTMLErrorMessage(responseBody)Note: Also update line 350 to use
responseBodyinstead ofcopiedResponseBody.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
core/changelog.md(1 hunks)core/providers/elevenlabs/elevenlabs.go(1 hunks)core/providers/mistral/mistral.go(5 hunks)core/providers/openai/openai.go(1 hunks)core/providers/utils/html_response_handler_test.go(1 hunks)core/providers/utils/utils.go(6 hunks)core/providers/vertex/errors.go(2 hunks)core/schemas/provider.go(1 hunks)transports/changelog.md(1 hunks)ui/app/workspace/providers/views/modelProviderKeysTableView.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- core/schemas/provider.go
- core/providers/elevenlabs/elevenlabs.go
- core/providers/openai/openai.go
- core/changelog.md
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
ui/app/workspace/providers/views/modelProviderKeysTableView.tsxcore/providers/utils/utils.gocore/providers/vertex/errors.gocore/providers/utils/html_response_handler_test.gocore/providers/mistral/mistral.gotransports/changelog.md
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/providers/utils/utils.gocore/providers/vertex/errors.gocore/providers/utils/html_response_handler_test.gocore/providers/mistral/mistral.go
📚 Learning: 2025-12-15T10:16:21.909Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/providers/huggingface/huggingface_test.go:12-63
Timestamp: 2025-12-15T10:16:21.909Z
Learning: In provider tests under core/providers/<provider>/*_test.go, do not require or flag the use of defer for Shutdown(); instead call client.Shutdown() at the end of each test function. This pattern appears consistent across all provider tests. Apply this rule only within this path; for other tests or resources, defer may still be appropriate.
Applied to files:
core/providers/utils/html_response_handler_test.go
🧬 Code graph analysis (3)
core/providers/vertex/errors.go (4)
core/schemas/bifrost.go (2)
BifrostError(461-470)ErrorField(479-486)ui/lib/types/logs.ts (2)
BifrostError(226-232)ErrorField(217-224)core/schemas/provider.go (2)
ErrProviderResponseEmpty(31-31)Provider(314-361)core/providers/utils/utils.go (2)
IsHTMLResponse(552-590)ExtractHTMLErrorMessage(597-665)
core/providers/utils/html_response_handler_test.go (1)
core/providers/utils/utils.go (3)
IsHTMLResponse(552-590)ExtractHTMLErrorMessage(597-665)HandleProviderAPIError(321-379)
core/providers/mistral/mistral.go (3)
core/providers/utils/utils.go (2)
CheckAndDecodeBody(540-548)NewBifrostOperationError(733-744)core/schemas/provider.go (2)
ErrProviderResponseDecode(29-29)ErrProviderResponseEmpty(31-31)core/schemas/bifrost.go (2)
BifrostError(461-470)ErrorField(479-486)
🔇 Additional comments (14)
transports/changelog.md (2)
1-1: Changelog entry aligns with PR objectives.The added entry clearly documents the main feature: HTML and empty response handling from providers.
2-2: Verify scope: mistral transcription feature relationship to this PR.The mistral transcription support entry appears tangential to the core PR objective (#1105), which focuses on HTML/empty response handling. Since the coding guidelines reference a stack of related PRs (#1114, #1113, #1112, #1105), please confirm whether this changelog entry belongs in this PR's section or should be attributed to a different stacked PR.
core/providers/utils/utils.go (4)
318-378: Good design: lazy HTML detection after JSON parse failure.The updated
HandleProviderAPIErrorcorrectly tries JSON parsing first and only falls back to HTML detection when unmarshalling fails. This avoids expensive regex operations on valid JSON responses—the common case.
387-397: Empty response check is a good addition.Early detection of empty responses with a clear error message improves debuggability. Using
strings.TrimSpacehandles whitespace-only responses correctly.
550-590: Well-structured HTML detection with appropriate short-circuits.The function correctly prioritizes the Content-Type header (most reliable), then falls back to body inspection. The early return for small bodies (
< 20bytes) is a sensible optimization.
435-455: Consistent HTML handling in generic response handler.The HTML detection path in
HandleProviderResponsecorrectly usesIsBifrostError: falseto indicate this is a provider-side issue, matching the pattern used inHandleProviderAPIError.core/providers/utils/html_response_handler_test.go (3)
10-80: Comprehensive test coverage for HTML detection.Good coverage of Content-Type detection, body-based detection, and edge cases (empty body, short body, JSON/plain text negatives).
82-192: Thorough extraction tests covering multiple HTML patterns.The tests cover title tags, heading hierarchy (h1/h2/h3), meta descriptions, and the fallback to visible text. The "Ignore script and style tags" test validates security-conscious content stripping.
194-282: Integration tests validate the lazy detection pattern.The test cases for
HandleProviderAPIErrorWithHTMLconfirm that HTML detection only triggers after JSON parsing fails, and that valid JSON responses bypass HTML detection entirely.core/providers/mistral/mistral.go (2)
302-333: HTML detection correctly moved to unmarshal failure path.This addresses the previous review feedback—HTML detection now only runs when JSON parsing fails, avoiding unnecessary regex overhead on valid responses.
500-512: Good function rename for clarity.Renaming from
processStreamEventtoprocessTranscriptionStreamEventbetter reflects the function's purpose and avoids potential confusion with other stream event processors.core/providers/vertex/errors.go (2)
34-52: Nil-safe empty response handling.The implementation correctly guards against nil
metabefore accessing its fields, addressing the previous review feedback.
54-72: Nil-safe HTML error handling appropriately prioritizes HTML detection.The nil check for
metais properly implemented. Vertex checks for HTML before attempting JSON parsing—this is a justified approach given that Vertex API responses are not always guaranteed to be JSON format and can include HTML content. While other providers may differ, provider-specific error handling patterns are reasonable when based on documented API behavior. No changes needed.ui/app/workspace/providers/views/modelProviderKeysTableView.tsx (1)
135-152: LGTM! Clean refactor of the key update logic.The ternary expression on line 142 is more concise while preserving the same behavior. The
size="md"prop addition is a reasonable styling adjustment.Verify that the
Switchcomponent from@/components/ui/switchsupports thesizeprop. If this is a custom component wrapper, ensure"md"is a valid size variant.#!/bin/bash # Check Switch component definition for size prop support fd -t f "switch.tsx" --exec cat {}
Pratham-Mishra04
force-pushed
the
12-16-feat_http_response_parsing_support_added
branch
from
eeb6bc7 to
d6c142a
Compare
Pratham-Mishra04
force-pushed
the
12-16-feat_http_response_parsing_support_added
branch
from
d6c142a to
f109532
Compare
Merge activity
|
Summary
Added support for handling HTML and empty responses from provider APIs, improving error handling and user experience when providers return unexpected response formats.
Changes
Type of change
Affected areas
How to test
Breaking changes
Security considerations
The HTML parsing is designed to be safe and only extract text content, avoiding any potential script execution or injection vulnerabilities.
Checklist